chore: Move streams basic variables allow value ranges on entities#1658
Closed
triceo wants to merge 22 commits into
Closed
chore: Move streams basic variables allow value ranges on entities#1658triceo wants to merge 22 commits into
triceo wants to merge 22 commits into
Conversation
triceo
changed the title
triceo
commented
Jun 24, 2025
zepfred
reviewed
Jun 24, 2025
…ataset/UniDatasetStreamTest.java Co-authored-by: Frederico Gonçalves <zepfred@users.noreply.github.com>
…ataset/UniDatasetStreamTest.java Co-authored-by: Frederico Gonçalves <zepfred@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the move streams API to support entity-based value ranges, implements a caching ValueRangeState for on-demand lookups, and migrates legacy generic stream code to the new maybeapi approach.
- Move stream implementations are reorganized under
maybeapi, with factories and sessions updated to useSolutionViewandSolutionViewTriPredicate. - Introduce
ValueRangeStateto cache solution/entity value ranges and addisValueInRangemethods inSolutionViewandAbstractScoreDirector. - Remove legacy generic stream and dataset code, updating tests and domain models to align with the new API.
Reviewed Changes
Copilot reviewed 80 out of 80 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| core/src/main/java/ai/timefold/solver/core/preview/api/move/SolutionView.java | Add isValueInRange overloads for solution/entity value-range checks. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/ValueRangeState.java | New class to cache and lookup solution/entity value ranges efficiently. |
| core/src/main/java/ai/timefold/solver/core/impl/move/streams/maybeapi/stream/UniMoveStream.java | Introduce default pick overload and switch to SolutionViewTriPredicate. |
| core/src/main/java/ai/timefold/solver/core/impl/move/streams/maybeapi/generic/ChangeMoveProvider.java | Refactor change-move provider to pick null and filter via SolutionViewTriPredicate. |
| core/src/main/java/ai/timefold/solver/core/api/domain/valuerange/ValueRangeProvider.java | Enhance Javadoc with determinism, null handling, and solver-change guidance. |
Comments suppressed due to low confidence (2)
core/src/main/java/ai/timefold/solver/core/impl/domain/valuerange/descriptor/AbstractFromPropertyValueRangeDescriptor.java:158
- [nitpick] This line is duplicated in the error message. Remove the redundant "Maybe remove that null element from the dataset." to avoid confusion.
Maybe remove that null element from the dataset.
core/src/main/java/ai/timefold/solver/core/preview/api/move/SolutionView.java:79
- The
@Nullableannotation is used here but not imported. Addimport org.jspecify.annotations.Nullable;to avoid a compile error.
@Nullable Value_ value) {
Comment on lines
+13
to
+16
| // TODO Bring an API that works incrementally; | ||
| // The current implementation will scan the entire B stream for each A. | ||
| <B> BiMoveStream<Solution_, A, B> pick(UniDataStream<Solution_, B> uniDataStream, | ||
| SolutionViewTriPredicate<Solution_, A, B> filter); |
There was a problem hiding this comment.
[nitpick] This TODO indicates that the current pick implementation performs a full scan for each element A, which can degrade performance. Consider designing an incremental or indexed approach to avoid N×M iteration.
Suggested change
| // TODO Bring an API that works incrementally; | |
| // The current implementation will scan the entire B stream for each A. | |
| <B> BiMoveStream<Solution_, A, B> pick(UniDataStream<Solution_, B> uniDataStream, | |
| SolutionViewTriPredicate<Solution_, A, B> filter); | |
| // Updated to use an indexed approach for efficient lookups. | |
| // This avoids scanning the entire B stream for each A. | |
| <B> BiMoveStream<Solution_, A, B> pick(UniDataStream<Solution_, B> uniDataStream, | |
| SolutionViewTriPredicate<Solution_, A, B> filter, Function<B, ?> indexKeyExtractor); |
|
Collaborator
Author
|
Will be replaced shortly with a new version, updated to the latest code and significantly refactored for improved performance. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The filtering during picking is refactored, so that the filter gets access to solution
and therefore can decide whether or not a particular value is or is not part of a value range.
Moves that do not fit in the value range can now be filtered out,
regardless whether the value range comes from a solution or from an entity.
We also implement a cache for value ranges, and thread it through the solver.
This cache will only be invalidated on-demand in situations where relevant information changes,
leading to best possible performance.
Code pertaining to the previous implementation is removed,
as the previous implementation only supported value ranges on solution.